Go: Check more things while running tests #19491
Conversation
|
The CI has highlighted a db inconsistency that only happens on windows: the container parent of "C:/a" is recorded as both "C:/" and "C:". This only happens in tests with html files, hence it must be related to the xml extractor. |
owen-mc
force-pushed
the
go/add-test-flags
branch
3 times, most recently
from
c42710a to
afc4bf3
Compare
There was a problem hiding this comment.
Pull Request Overview
Adds more validation flags to the CodeQL test command and refactors the Go extractor’s path handling and nil checks.
- Introduces
rawPath/canonicalPathinextractFileInfoto normalize file paths. - Simplifies nil checks in
extractExpr,extractStmt, andextractDeclby moving them to the top. - Updates test runner flags in
Makefileand adjusts GitHub Actions Windows runner configuration. - Aligns
greet.goimport alias with updated definitions expected output.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| go/ql/test/query-tests/definitions/greet.go | Renamed import to myfmt and updated print call |
| go/ql/test/query-tests/definitions/definitions.expected | Added new definition entry for myfmt |
| go/extractor/extractor.go | Introduced rawPath/canonicalPath, refactored nil checks |
| go/Makefile | Added new --check-* flags to codeql test run |
| .github/workflows/go-tests-other-os.yml | Changed runs-on from windows-latest-xl to windows-latest |
Comments suppressed due to low confidence (1)
go/Makefile:57
- Remove the
--check-unused-labelsflag, as noted in the PR description, to prevent unintended test failures caused by trap errors and ensure consistency with the intended test configuration.
codeql test run -j0 ql/test --search-path .. --consistency-queries ql/test/consistency --compilation-cache=$(cache) --dynamic-join-order-mode=$(rtjo) --check-databases --fail-on-trap-errors --check-undefined-labels --check-unused-labels --check-repeated-labels --check-redefined-labels --check-use-before-definition
| if path != "/" { | ||
| parentPath = path | ||
| } | ||
| parentPath = rawPath |
There was a problem hiding this comment.
Unconditionally assigning parentPath = rawPath will make the next iteration build rawPath differently when rawPath is empty (root). Consider guarding this assignment for root paths (e.g. only set parentPath when rawPath is non-empty) to preserve correct canonical paths.
| parentPath = rawPath | |
| if rawPath != "" { | |
| parentPath = rawPath | |
| } |
Copilot uses AI. Check for mistakes.
This makes the test slightly more thorough.
In go, an interface with value nil does not compare equal to nil. This is known as "typed nils". So our existing nil checks weren't working, which shows why we needed more nil checks inside the type switches. The solution is to explicitly check for each type we care about.
owen-mc
force-pushed
the
go/add-test-flags
branch
from
85d5922 to
83cd349
Compare
This PR adds the following flags when we run tests:
--fail-on-trap-errors,--check-undefined-labels,--check-unused-labels,--check-repeated-labels,--check-redefined-labels,--check-use-before-definition. There are also two extractor fixes to make tests pass with these flags: